Fix build with boost ASIO 1.87. Support boost 1.66+ #9628

vtnerd · 2024-12-17T22:02:37Z

This should compile with Boost 1.73-1.87. I have only tested on Boost 1.74 and Boost 1.87 (primarily unit tests). I will mark the places where I know changes need to be made for older boost versions. I will clear up all such errors by manually downloading Boost 1.66 and testing.

One problem I have with supporting Boost 1.66-1.69 is that most types require an io_context object, whereas Boost 1.70+ have relaxed this requirement to an executor object. I will show an example of where it matters; it will be easy to break older Boost versions unintentionally.

EDIT:
This is currently marked as a draft, because the depends build will fail until #9162 gets merged (which is also marked as draft).

contrib/epee/include/net/abstract_tcp_server2.inl

contrib/epee/src/net_ssl.cpp

contrib/epee/src/net_helper.cpp

tobtoht · 2024-12-17T22:27:36Z

(This needs #9162 to pass depends CI. I will undraft that PR after the next round of merges.)

vtnerd · 2024-12-18T02:55:16Z

The latest force push should work with Boost 1.66-1.87. Something funky happened with the tests, so I may push a dummy change later to see what happens.

tobtoht · 2024-12-18T10:49:02Z

https://github.com/tobtoht/monero/actions/runs/12390979989/job/34587144842#step:5:1281

Looks like boost::asio::io_service is also used in src/device_trezor/trezor.

contrib/epee/include/net/abstract_tcp_server2.h

vtnerd · 2024-12-20T17:19:14Z

Fore pushed some changes:

Reverted changes to query code, they are basically same style with new interface
Switched to io_context_t as @jeffro256 suggested
Fixed Trezor build.

vtnerd · 2024-12-20T19:23:24Z

Force pushed twice with updates:

Added a missing include, but it doesn't matter as the depends builds is using Boost 1.64 which won't work
Reverted some whitespace changes that were made when making the patch.

vtnerd · 2024-12-20T19:38:54Z

Force pushed an include fix.

vtnerd · 2024-12-20T23:02:39Z

This patch looks ready. The depends CI will all fail until #9162 gets merged. I will leave as draft for now for that reason.

selsta

README needs an update to indicate the new minimum version

0xFFFC0000 · 2024-12-25T06:31:32Z

Once this is ready for review, please remove Draft status.

vtnerd · 2024-12-25T23:25:08Z

Force pushed:

A rebase of recent master merges.
Reverted ci: brew: pin boost to 1.85 #9625 (no longer needed)
Reverted ci: msys2: pin boost to 1.86.0 #9623 (no longer needed)
Requires depends: boost: update to 1.84.0 #9162 for depends CI to pass

Likely this will get removed as draft, assuming some of the tests cycle green.

vtnerd · 2024-12-25T23:46:03Z

README needs an update to indicate the new minimum version

Forgot to update this in the last force push, so another quick push with this change.

vtnerd · 2024-12-26T14:19:56Z

Now ready for review.

jeffro256 · 2024-12-27T00:05:46Z

Getting a ton of build errors across many platforms:

fatal error: boost/asio/io_context.hpp: No such file or directory

jeffro256 · 2024-12-27T00:09:50Z

The dependencies system seems to still be pulling Boost 1.64 instead of 1.66 on these failed platforms

selsta · 2024-12-27T00:21:25Z

@jeffro256 requires #9162 to be merged first

jeffro256 · 2024-12-27T00:25:42Z

Sorry, my bad. Should've read farther up in the comments

0xFFFC0000

From technical perspective, everything is great, and I don't see any specific issues.

The only remaining thing is I have to fetch #9162 and compile, and run all the tests.

jeffro256

A couple actual comments, but mostly nitpicks

jeffro256 · 2024-12-27T00:29:08Z

contrib/epee/include/net/abstract_tcp_server2.h

-    using io_context_t = boost::asio::io_service;
-    using strand_t = boost::asio::io_service::strand;
+    using io_context_t = boost::asio::io_context;
+    using strand_t = boost::asio::io_context::strand;


Suggested change

using strand_t = boost::asio::io_context::strand;

using strand_t = io_context_t::strand;

jeffro256 · 2024-12-27T00:29:46Z

contrib/epee/include/net/abstract_tcp_server2.h

@@ -306,7 +308,7 @@ namespace net_utils
    virtual bool close();
    virtual bool call_run_once_service_io();
    virtual bool request_callback();
-    virtual boost::asio::io_service& get_io_service();
+    virtual boost::asio::io_context& get_io_context();


Suggested change

virtual boost::asio::io_context& get_io_context();

virtual io_context_t& get_io_context();

jeffro256 · 2024-12-27T16:34:10Z

contrib/epee/include/net/abstract_tcp_server2.inl

@@ -1767,7 +1776,7 @@ namespace net_utils
    if (r)
    {
      new_connection_l->get_context(conn_context);
-      //new_connection_l.reset(new connection<t_protocol_handler>(io_service_, m_config, m_sock_count, m_pfilter));
+      //new_connection_l.reset(new connection<t_protocol_handler>(io_context_, m_config, m_sock_count, m_pfilter));


Why not just remove this line?

jeffro256 · 2024-12-27T16:47:36Z

contrib/epee/include/net/abstract_tcp_server2.inl

    try
    {
      //resolving ipv4 address as ipv6 throws, catch here and move on
-      iterator = resolver.resolve(query, resolve_error);
+      results = resolver.resolve(


I think we should remove DNS resolution from this function completely. This is supposed to be an async function, but this DNS call is not async, which I'm sure degrades the performance of the main I/O loop. Also, the one place in the codebase that calls connect_async() (p2p) I think is calling it with raw IPv4 and IPv6 addresses, so it shouldn't even need DNS resolution.

jeffro256 · 2024-12-27T16:58:18Z

contrib/epee/include/net/abstract_tcp_server2.inl

    try
    {
      //resolving ipv4 address as ipv6 throws, catch here and move on
-      iterator = resolver.resolve(query, resolve_error);
+      results = resolver.resolve(


Same here as other comment: the one place this is called, we use raw IP addresses

jeffro256 · 2024-12-27T17:15:12Z

tests/net_load_tests/net_load_tests.h

@@ -32,7 +32,7 @@

 #include <atomic>

-#include <boost/asio/io_service.hpp>
+#include <boost/asio/io_context.hpp>


Should this header include even be here in the first place?

jeffro256 · 2024-12-27T19:27:12Z

contrib/epee/src/connection_basic.cpp

 	m_want_close_connection(false),
 	m_was_shutdown(false),
 	m_is_multithreaded(false),
 	m_ssl_support(ssl_support)
 {
 	// add nullptr checks if removed
-	assert(m_state != nullptr); // release runtime check in get_context
+	assert(m_state != nullptr); // release runtime check in get_contextp


jeffro256 · 2024-12-27T19:27:51Z

contrib/epee/src/connection_basic.cpp


-        socket_.next_layer() = std::move(sock);
+  socket_.next_layer() = std::move(sock);  


what's happening with the whitespace here?

jeffro256 · 2024-12-27T19:36:05Z

contrib/epee/src/net_helper.cpp

+			results = resolver.resolve(
+				boost::asio::ip::tcp::v6(), addr, port, boost::asio::ip::tcp::resolver::canonical_name
+			);
+			if (results.empty())


I think that this overload of resolve() shouldn't ever return an empty range because otherwise, it will throw a boost exception. That would make this if branch unreachable.

jeffro256 · 2024-12-27T19:37:44Z

contrib/epee/src/net_helper.cpp

 				throw boost::system::system_error{boost::asio::error::fault, "Failed to resolve " + addr};
 		}

 		//////////////////////////////////////////////////////////////////////////

-		struct new_connection


Was the move of this struct declaration purely for readability purposes?

vtnerd commented Dec 17, 2024

View reviewed changes

contrib/epee/include/net/abstract_tcp_server2.inl Outdated Show resolved Hide resolved

contrib/epee/src/net_ssl.cpp Outdated Show resolved Hide resolved

contrib/epee/src/net_helper.cpp Outdated Show resolved Hide resolved

selsta added the build system label Dec 18, 2024

vtnerd force-pushed the fix/boost_1.87_squashed branch from 8c98c45 to e88e61d Compare December 18, 2024 02:51

vtnerd mentioned this pull request Dec 18, 2024

Add support for upcoming Boost 1.87.0 (Boost.Asio removals) #9596

Open

jeffro256 reviewed Dec 18, 2024

View reviewed changes

contrib/epee/include/net/abstract_tcp_server2.h Outdated Show resolved Hide resolved

vtnerd mentioned this pull request Dec 18, 2024

Update deprecated features removed in Boost ASIO 1.87 vtnerd/monero-lws#153

Open

vtnerd force-pushed the fix/boost_1.87_squashed branch from e88e61d to f185187 Compare December 20, 2024 17:18

vtnerd force-pushed the fix/boost_1.87_squashed branch 2 times, most recently from 1c62d08 to da62d98 Compare December 20, 2024 19:22

vtnerd force-pushed the fix/boost_1.87_squashed branch from da62d98 to a0ce1e9 Compare December 20, 2024 19:38

selsta added the pending review label Dec 20, 2024

selsta reviewed Dec 23, 2024

View reviewed changes

vtnerd force-pushed the fix/boost_1.87_squashed branch 2 times, most recently from 625f370 to 0452910 Compare December 25, 2024 23:17

Fix build with boost ASIO 1.87. Support boost 1.66+

ad7eaaa

vtnerd force-pushed the fix/boost_1.87_squashed branch from 0452910 to ad7eaaa Compare December 25, 2024 23:45

vtnerd marked this pull request as ready for review December 26, 2024 14:19

0xFFFC0000 reviewed Dec 27, 2024

View reviewed changes

jeffro256 suggested changes Dec 27, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix build with boost ASIO 1.87. Support boost 1.66+ #9628

Fix build with boost ASIO 1.87. Support boost 1.66+ #9628

vtnerd commented Dec 17, 2024 •

edited

Loading

tobtoht commented Dec 17, 2024

vtnerd commented Dec 18, 2024

tobtoht commented Dec 18, 2024

vtnerd commented Dec 20, 2024 •

edited

Loading

vtnerd commented Dec 20, 2024

vtnerd commented Dec 20, 2024

vtnerd commented Dec 20, 2024

selsta left a comment

0xFFFC0000 commented Dec 25, 2024

vtnerd commented Dec 25, 2024

vtnerd commented Dec 25, 2024

vtnerd commented Dec 26, 2024

jeffro256 commented Dec 27, 2024

jeffro256 commented Dec 27, 2024 •

edited

Loading

selsta commented Dec 27, 2024

jeffro256 commented Dec 27, 2024

0xFFFC0000 left a comment •

edited

Loading

jeffro256 left a comment

jeffro256 Dec 27, 2024

jeffro256 Dec 27, 2024

jeffro256 Dec 27, 2024

jeffro256 Dec 27, 2024

jeffro256 Dec 27, 2024

jeffro256 Dec 27, 2024

jeffro256 Dec 27, 2024

jeffro256 Dec 27, 2024

jeffro256 Dec 27, 2024

jeffro256 Dec 27, 2024

	using strand_t = boost::asio::io_context::strand;
	using strand_t = io_context_t::strand;

	virtual boost::asio::io_context& get_io_context();
	virtual io_context_t& get_io_context();


		socket_.next_layer() = std::move(sock);
		socket_.next_layer() = std::move(sock);

Fix build with boost ASIO 1.87. Support boost 1.66+ #9628

Are you sure you want to change the base?

Fix build with boost ASIO 1.87. Support boost 1.66+ #9628

Conversation

vtnerd commented Dec 17, 2024 • edited Loading

tobtoht commented Dec 17, 2024

vtnerd commented Dec 18, 2024

tobtoht commented Dec 18, 2024

vtnerd commented Dec 20, 2024 • edited Loading

vtnerd commented Dec 20, 2024

vtnerd commented Dec 20, 2024

vtnerd commented Dec 20, 2024

selsta left a comment

Choose a reason for hiding this comment

0xFFFC0000 commented Dec 25, 2024

vtnerd commented Dec 25, 2024

vtnerd commented Dec 25, 2024

vtnerd commented Dec 26, 2024

jeffro256 commented Dec 27, 2024

jeffro256 commented Dec 27, 2024 • edited Loading

selsta commented Dec 27, 2024

jeffro256 commented Dec 27, 2024

0xFFFC0000 left a comment • edited Loading

Choose a reason for hiding this comment

jeffro256 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vtnerd commented Dec 17, 2024 •

edited

Loading

vtnerd commented Dec 20, 2024 •

edited

Loading

jeffro256 commented Dec 27, 2024 •

edited

Loading

0xFFFC0000 left a comment •

edited

Loading